Provide support for commands by gatesn · Pull Request #21 · TypeFox/monaco-languageclient

gatesn · 2017-08-11T13:17:11Z

This implements a basic applyEdit operation (more advanced ones would accept the editor and preserve / manipulate the current selections), as well as registering commands against a given editor.

Fixes #11
Fixes #12

gatesn · 2017-08-11T13:33:41Z

Actually, having looked at that disgusting applyEdit function, we should iterate over the changes and then apply them all in bulk. Otherwise the undo stack looks a bit odd.

akosyakov

Thanks for looking into it. Please see comments.

Most important to have a proof that it is working, e.g. with the JSON example server. Please add a command that does workspace edits to demonstrate it.

akosyakov · 2017-08-11T13:31:02Z


+    public applyEdit(workspaceEdit: WorkspaceEdit): Thenable<boolean> {
+        let applied = true;
+        if (workspaceEdit.documentChanges) {


please use asWorkspaceEdit, applying documentChanges is not enough

akosyakov · 2017-08-11T13:31:08Z

        return this.onDidChangeTextDocumentEmitter.event;
    }

+    public applyEdit(workspaceEdit: WorkspaceEdit): Thenable<boolean> {


please return Promise

akosyakov · 2017-08-11T13:39:01Z

+        let applied = true;
+        if (workspaceEdit.documentChanges) {
+            for (const change of workspaceEdit.documentChanges) {
+                if (change.textDocument.version && change.textDocument.version >= 0) {


Are these checks necessary?

akosyakov · 2017-08-11T13:39:19Z

+                        applied = false;
+                    }
+                } else {
+                    applied = false;


Is not the whole change should be applied or rejected? It seems that changes can be applied partially by this method.

akosyakov · 2017-08-11T13:41:37Z

+                if (change.textDocument.version && change.textDocument.version >= 0) {
+                    const textDocument = this.documents.get(change.textDocument.uri);
+                    if (textDocument && textDocument.version === change.textDocument.version) {
+                        monaco.editor.getModel(monaco.Uri.parse(textDocument.uri)).pushEditOperations(


Should not you check that the model is defined?

akosyakov · 2017-08-11T13:42:00Z

+
+export class MonacoCommands implements Commands {
+
+    public constructor(private _editor: monaco.editor.IStandaloneCodeEditor) { }


protected readonly editor

akosyakov · 2017-08-11T13:42:38Z

+    public constructor(private _editor: monaco.editor.IStandaloneCodeEditor) { }
+
+    public registerCommand(command: string, callback: (...args: any[]) => any, thisArg?: any): Disposable {
+        return (this._editor as any)._commandService.addCommand(command, {


please introduce augmenting typings for internals instead of using any

Could you explain what you mean here? Not sure I'm familiar with this technique.

I meant that you should add d.ts. file that augments monaco module add exposes _commandService, look at https://github.com/theia-ide/theia/blob/master/packages/monaco/src/typings/monaco/index.d.ts.

akosyakov · 2017-08-11T13:45:37Z

+    public registerCommand(command: string, callback: (...args: any[]) => any, thisArg?: any): Disposable {
+        return (this._editor as any)._commandService.addCommand(command, {
+            handler: (id: string, ...args: any[]) => {
+                console.log("Executing command", command, id, args);


Won't it pollute console?

Sorry, left in from testing!

akosyakov · 2017-08-11T13:46:06Z

+        } else {
+            applied = false;
+        }
+        return Promise.resolve(applied);


Could you use high-level functions as reduce to get rid of if-else cascade?

gatesn · 2017-08-11T15:49:41Z

I've made the changes but not written an example. Any ideas on what to use as an example?!

akosyakov · 2017-08-25T09:28:33Z

@gatesn the same here please use rebase instead of merge

akosyakov · 2017-08-25T09:36:22Z

I like changes but there should be a proof that it is working: tests or use of it in the example.

What if the JSON server provides:

a command that converts a property name at the position to the lower via applyEdit
some way to trigger it for example via the code action

It should not be sophisticated, but only demonstrate that everything works together.

gatesn · 2017-08-27T21:48:28Z

@akosyakov I created an example that provides a code action for any document in all positions. This action triggers a server-side command that upper cases the entire document.

akosyakov · 2017-08-29T07:35:58Z

@gatesn I've tested this PR and it works nicely, thank you! I've opened a new PR against your, please merge it and after that, I will approve this PR.

Next time you can work in the branch directly in this repo since you have the write access instead of working on the fork. It makes easy to push new commits to PRs.

…age server Signed-off-by: Nicholas Gates <ngates@palantir.com>

gatesn · 2017-08-29T15:33:24Z

@akosyakov merged your changes, thanks

gatesn · 2017-08-29T16:47:57Z

Do you think we could cut a release after this merges?

akosyakov · 2017-08-30T04:52:25Z

@gatesn LGTM, thank you again!

akosyakov · 2017-08-30T04:52:33Z

Do you think we could cut a release after this merges?

yes, we can make a release. Should we migrate to monaco 0.10.0 before the release?

akosyakov · 2017-08-30T04:53:29Z

btw i wonder why github cannot recognize your account as an author, see: https://github.com/TypeFox/monaco-languageclient/commits/master

gatesn · 2017-08-30T07:00:14Z

Probably no 0.10.0 given the breaking changes around Code actions. We might need to investigate to see what that means.

gatesn · 2017-08-30T07:00:51Z

And it seems my git setup is different on different laptop :/

akosyakov · 2017-08-30T15:02:55Z

@gatesn 0.2.0 is published with 0.10.0 Monaco support thanks to @rcjsuen

Pull request TypeFox#21 added support for the workspace/applyEdit capability so it should be stated as such in the WorkspaceClientCapabilities. Signed-off-by: Remy Suen <remy.suen@gmail.com>

Pull request #21 added support for the workspace/applyEdit capability so it should be stated as such in the WorkspaceClientCapabilities. Signed-off-by: Remy Suen <remy.suen@gmail.com>

gatesn force-pushed the ngates/lsp-commands branch from 2011a25 to 7471a0c Compare August 11, 2017 13:30

akosyakov suggested changes Aug 11, 2017

View reviewed changes

gatesn force-pushed the ngates/lsp-commands branch from 7471a0c to 47cc839 Compare August 11, 2017 14:44

akosyakov assigned gatesn Aug 25, 2017

gatesn force-pushed the ngates/lsp-commands branch 2 times, most recently from 5800486 to 6679516 Compare August 27, 2017 21:47

gatesn force-pushed the ngates/lsp-commands branch 2 times, most recently from 1573c3d to fad66e0 Compare August 28, 2017 14:56

akosyakov mentioned this pull request Aug 29, 2017

Fix triggering code action from the horizontal ruler #11

Closed

Add support for CodeActions triggering commands provided by the langu…

0cd9823

…age server Signed-off-by: Nicholas Gates <ngates@palantir.com>

gatesn force-pushed the ngates/lsp-commands branch from 4e2162c to 0cd9823 Compare August 29, 2017 15:32

akosyakov approved these changes Aug 30, 2017

View reviewed changes

akosyakov merged commit efe943c into TypeFox:master Aug 30, 2017

rcjsuen mentioned this pull request Feb 11, 2018

State that the WorkspaceClientCapabilites's applyEdit capability is enabled #55

Merged

rcjsuen mentioned this pull request Feb 12, 2018

Update WorkspaceClientCapabilities to state that WorkspaceEdit's documentChanges is supported #56

Merged


		export class MonacoCommands implements Commands {

		public constructor(private _editor: monaco.editor.IStandaloneCodeEditor) { }

Uh oh!

Uh oh!

Conversation

gatesn commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatesn commented Aug 11, 2017

Uh oh!

akosyakov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatesn commented Aug 11, 2017

Uh oh!

akosyakov commented Aug 25, 2017

Uh oh!

akosyakov commented Aug 25, 2017

Uh oh!

gatesn commented Aug 27, 2017

Uh oh!

akosyakov commented Aug 29, 2017

Uh oh!

gatesn commented Aug 29, 2017

Uh oh!

gatesn commented Aug 29, 2017

Uh oh!

akosyakov commented Aug 30, 2017

Uh oh!

akosyakov commented Aug 30, 2017

Uh oh!

akosyakov commented Aug 30, 2017

Uh oh!

gatesn commented Aug 30, 2017

Uh oh!

gatesn commented Aug 30, 2017

Uh oh!

akosyakov commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gatesn commented Aug 11, 2017 •

edited

Loading